-
Notifications
You must be signed in to change notification settings - Fork 386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#3081] improvement(core): Remove hack in BaseCatalog.java #3166
Conversation
@noidname01 Thanks a lot for your contribution. I think the issue is misleading, this part of the code is required for the some advanced users, we just leave a back door for such users without documentation. |
@jerryshao Sure, so this issue is not necessary? |
I think what you need to do is to improve the comment to avoid saying that this is a hacky way. I think why @justinmclean created this issue is that he uses some tools to do code scan, that tool may trigger a warning when finding this keyword. |
OK, Got it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks a lot for your contribution.
### What changes were proposed in this pull request? Modify the description of `CATALOG_OPERATION_IMPL`, avoid the use of "hack" ### Why are the changes needed? Fix: #3081 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? No need to test --------- Co-authored-by: TimWang <[email protected]>
…che#3166) ### What changes were proposed in this pull request? Modify the description of `CATALOG_OPERATION_IMPL`, avoid the use of "hack" ### Why are the changes needed? Fix: apache#3081 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? No need to test --------- Co-authored-by: TimWang <[email protected]>
What changes were proposed in this pull request?
Modify the description of
CATALOG_OPERATION_IMPL
, avoid the use of "hack"Why are the changes needed?
Fix: #3081
Does this PR introduce any user-facing change?
No
How was this patch tested?
No need to test